ARROW-9790: [Rust][Parquet]: Increase test coverage in arrow_reader.rs #8009
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alamb
commented
Aug 19, 2020
Contributor
Author
There was a problem hiding this comment.
This is the basic idea -- drive the test with a table that is shared between bool, utf, and fixed_len_binary_array types
Member
There was a problem hiding this comment.
Since you felt the need to add a comment explaining the values, a tiny suggestion is to declare all members of TestOptions as pub and use
TestOptions {
num_row_groups: 3,
num_rows: 25,
record_batch_size: 5,
num_iterations: 50,
}
instead, to increase readability of the values' meaning.
Contributor
Author
There was a problem hiding this comment.
This is a good idea -- I will do so before turning this into a full on PR for review
alamb
force-pushed
the
alamb/ARROW-9790-test-consolidation
branch
2 times, most recently
from
3967510 to
f149500
Compare
alamb
force-pushed
the
alamb/ARROW-9790-test-consolidation
branch
from
f149500 to
bc384b3
Compare
alamb
commented
Aug 30, 2020
| } | ||
|
|
||
| #[test] | ||
| fn test_bool_single_column_reader_test_batch_size_divides_into_row_group_size() { |
Contributor
Author
There was a problem hiding this comment.
The coverage of these tests was moved into two table entries here: https://github.com/apache/arrow/pull/8009/files#diff-4c103051156d7b901fad8d9e26104932R393
andygrove
approved these changes
Sep 12, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I added two test cases in #8007, which increased coverage. However, upon further review, I noticed the choice of parameters to hit edge conditions didn't cover the string data types.
Rather than adding a bunch more copies of basically the same test to add new parameters for different tests, I instead propose using the same set of parameters for all data types and drive the tests using a table in this PR.
It makes the test logic slightly harder to follow, in my opinion, but it does increase coverage